Clarify stability guarantee for lifetimes in enum discriminants #104299

mkrasnitski · 2022-11-11T19:17:16Z

Since std::mem::Discriminant erases lifetimes, it should be clarified that changing the concrete value of a lifetime parameter does not change the value of an enum discriminant for a given variant. This is useful as it guarantees that it is safe to transmute Discriminant<Foo<'a>> to Discriminant<Foo<'b>> for any combination of 'a and 'b. This also holds for type-generics as long as the type parameters do not change, e.g. Discriminant<Foo<T, 'a>> can be transmuted to Discriminant<Foo<T, 'b>>.

Side note: Is what I've written actually enough to imply soundness (or rather codify it), or should it specifically be spelled out that it's OK to transmute in the above way?

rustbot · 2022-11-11T19:17:23Z

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2022-11-11T19:17:25Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

CraftSpider · 2022-11-11T20:51:01Z

I'm not libs or anything, but I'd think this is already implied to be okay by the decision on #101520 - lifetimes are guaranteed to never change a type's layout. Not against documenting it, but just thought of that PR when I saw this one.

thomcc · 2022-11-12T02:04:12Z

I think I agree here, but don't feel particularly qualified to make a decision about documentation like this. Lets see, does the new bot allow this?

r? @rust-lang/lang

mkrasnitski · 2022-11-14T20:05:07Z

@CraftSpider That PR you linked simply allows the compiler to accept such transmutes in all cases (previously it wouldn't work if generic type parameters were present), but it says nothing about their soundness. If a type has some data tied to its lifetime parameter, there are surely times when transmuting the lifetime is unsound (by changing when the data gets dropped). But types like Discriminant<Foo<'a>> and PhantomData<'a> do not carry any data tied to their lifetime parameter, so transmuting the value of that parameter will always be sound, as far as I understand. Maybe the scope of this documentation change should be expanded to encompass all such types.

mkrasnitski · 2023-02-13T01:34:48Z

Bumping and re-requesting review as it's been several months with no activity, and also the currently assigned reviewer is no longer t-lang.

r? @rust-lang/lang

tmandry · 2023-03-02T05:34:42Z

@rfcbot fcp merge

This PR documents that the discriminants of enums that are generic over a lifetime do not change when only the concrete value of the lifetime changes.

It does look related to the decision in #101520, but that was about transmutes, and this is about the value of enum discriminants.

I don't see how we could sensibly do anything different here, so I think we should go ahead and document this.

rfcbot · 2023-03-02T05:34:44Z

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

scottmcm · 2023-03-07T20:18:26Z

I agree we can't do anything different here -- any attempt would presumably hit the same lifetime soundness issue that's blocking arbitrary specialization -- so it makes sense to guarantee it.

@rfcbot reviewed

That said, I wonder if we could be more direct about "will not change" in the docs somehow, since "the discriminant" has a couple different possible meanings (variant index, encoded discriminant, declared discriminant, mem::Discriminant).

Quick attempt:

The value of a mem::Discriminant<T> is independent of any lifetimes in T. As such, reading or writing a Discriminant<Foo<'a>> as a Discriminant<Foo<'b>> (whether via transmute or otherwise) is sound. (Note that this is not true for other kinds of generic parameters; Discriminant<Foo<A>> and Discriminant<Foo<B>> might be incompatible.)

mkrasnitski · 2023-03-13T05:33:55Z

@scottmcm Thank you for the suggestion - your wording is much clearer than what I'd originally written! I've adapted it and pushed a new commit.

nikomatsakis · 2023-03-14T18:23:19Z

@rfcbot reviewed

We are pretty committed to lifetime erasure :)

rfcbot · 2023-03-14T18:23:26Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2023-03-24T18:25:15Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

mkrasnitski · 2023-04-02T21:04:33Z

Anything blocking this from being merged? @tmandry

mkrasnitski · 2023-09-08T00:38:37Z

Bumping - @tmandry, would appreciate a merge :)

oli-obk · 2023-09-08T09:37:57Z

@bors r+ rollup

FCP finished and the wording got adjusted as requested

bors · 2023-09-08T09:37:59Z

📌 Commit 571b0fe has been approved by oli-obk

It is now in the queue for this repository.

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#104299 (Clarify stability guarantee for lifetimes in enum discriminants) - rust-lang#115088 (Fix Step Skipping Caused by Using the `--exclude` Option) - rust-lang#115201 (rustdoc: list matching impls on type aliases) - rust-lang#115633 (Lint node for `PRIVATE_BOUNDS`/`PRIVATE_INTERFACES` is the item which names the private type) - rust-lang#115638 (`-Cllvm-args` usability improvement) - rust-lang#115643 (fix: return early when has tainted in mir-lint) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#104299 - mkrasnitski:discriminant-transmute-docs, r=oli-obk Clarify stability guarantee for lifetimes in enum discriminants Since `std::mem::Discriminant` erases lifetimes, it should be clarified that changing the concrete value of a lifetime parameter does not change the value of an enum discriminant for a given variant. This is useful as it guarantees that it is safe to transmute `Discriminant<Foo<'a>>` to `Discriminant<Foo<'b>>` for any combination of `'a` and `'b`. This also holds for type-generics as long as the type parameters do not change, e.g. `Discriminant<Foo<T, 'a>>` can be transmuted to `Discriminant<Foo<T, 'b>>`. Side note: Is what I've written actually enough to imply soundness (or rather codify it), or should it specifically be spelled out that it's OK to transmute in the above way?

lcnr · 2023-11-17T10:49:16Z

library/core/src/mem/mod.rs

@@ -1118,6 +1118,11 @@ impl<T> fmt::Debug for Discriminant<T> {
 ///
 /// [Reference]: ../../reference/items/enumerations.html#custom-discriminant-values-for-fieldless-enumerations
 ///
+/// The value of a [`Discriminant<T>`] is independent of any *lifetimes* in `T`. As such, reading


Suggested change

/// The value of a [`Discriminant<T>`] is independent of any *lifetimes* in `T`. As such, reading

/// The value of a [`Discriminant<T>`] is independent of any **free** *lifetimes* in `T`. As such, reading

It can be unsound to transmute Discrimant<MyType<for<'a, 'b> fn(Inv<'a>, Inv<'b>)>> to Discrimant<MyType<for<'a> fn(Inv<'a>, Inv<'a>)>>.

(as a general procedural thought, I feel like this decision should have been at least co-owned by t-types)

opened #118006

My apologies about not looping in t-types. That team didn't exist when I first opened this PR. However, I agree they should have been added while this PR was still under review.

Pkgsrc changes: * Remove NetBSD-8 support (embedded LLVm requires newer C++ than what is in -8; it's conceivable that this could still build with an external LLVM) * undo powerpc 9.0 file naming tweak, since we no longer support -8. * Remove patch to LLVM for powerpc now included by upstream. * Minor adjustments, checksum changes etc. Upstream changes: Version 1.74.1 (2023-12-07) =========================== - [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM] (rust-lang/rust#118464) - [Clarify guarantees for std::mem::discriminant] (rust-lang/rust#118006) - [Fix some subtyping-related regressions] (rust-lang/rust#116415) Version 1.74.0 (2023-11-16) ========================== Language -------- - [Codify that `std::mem::Discriminant<T>` does not depend on any lifetimes in T] (rust-lang/rust#104299) - [Replace `private_in_public` lint with `private_interfaces` and `private_bounds` per RFC 2145] (rust-lang/rust#113126) Read more in [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html). - [Allow explicit `#[repr(Rust)]`] (rust-lang/rust#114201) - [closure field capturing: don't depend on alignment of packed fields] (rust-lang/rust#115315) - [Enable MIR-based drop-tracking for `async` blocks] (rust-lang/rust#107421) Compiler -------- - [stabilize combining +bundle and +whole-archive link modifiers] (rust-lang/rust#113301) - [Stabilize `PATH` option for `--print KIND=PATH`] (rust-lang/rust#114183) - [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`] (rust-lang/rust#115644) - [Promote loongarch64-unknown-none* to Tier 2] (rust-lang/rust#115368) - [Add `i686-pc-windows-gnullvm` as a tier 3 target] (rust-lang/rust#115687) Libraries --------- - [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err] (rust-lang/rust#98704) - [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`] (rust-lang/rust#111278) - [impl Step for IP addresses] (rust-lang/rust#113748) - [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`] (rust-lang/rust#114041) - [`impl TryFrom<char> for u16`] (rust-lang/rust#114065) - [Stabilize `io_error_other` feature] (rust-lang/rust#115453) - [Stabilize the `Saturating` type] (rust-lang/rust#115477) - [Stabilize const_transmute_copy] (rust-lang/rust#115520) Stabilized APIs --------------- - [`core::num::Saturating`] (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html) - [`impl From<io::Stdout> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStdout%3E-for-Stdio) - [`impl From<io::Stderr> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-for-Stdio) - [`std::ffi::OsString::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsString::into_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes) - [`std::ffi::OsStr::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsStr::as_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes) - [`std::io::Error::other`] (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other) - [`impl TryFrom<char> for u16`] (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom%3Cchar%3E-for-u16) - [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E) - [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E) - [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E) - [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E) These APIs are now stable in const contexts: - [`core::mem::transmute_copy`] (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html) - [`str::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii) - [`[u8]::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii) Cargo ----- - [fix: Set MSRV for internal packages] (rust-lang/cargo#12381) - [config: merge lists in precedence order] (rust-lang/cargo#12515) - [fix(update): Clarify meaning of --aggressive as --recursive] (rust-lang/cargo#12544) - [fix(update): Make `-p` more convenient by being positional] (rust-lang/cargo#12545) - [feat(help): Add styling to help output ] (rust-lang/cargo#12578) - [feat(pkgid): Allow incomplete versions when unambigious] (rust-lang/cargo#12614) - [feat: stabilize credential-process and registry-auth] (rust-lang/cargo#12649) - [feat(cli): Add '-n' to dry-run] (rust-lang/cargo#12660) - [Add support for `target.'cfg(..)'.linker`] (rust-lang/cargo#12535) - [Stabilize `--keep-going`] (rust-lang/cargo#12568) - [feat: Stabilize lints] (rust-lang/cargo#12648) Rustdoc ------- - [Add warning block support in rustdoc] (rust-lang/rust#106561) - [Accept additional user-defined syntax classes in fenced code blocks] (rust-lang/rust#110800) - [rustdoc-search: add support for type parameters] (rust-lang/rust#112725) - [rustdoc: show inner enum and struct in type definition for concrete type] (rust-lang/rust#114855) Compatibility Notes ------------------- - [Raise minimum supported Apple OS versions] (rust-lang/rust#104385) - [make Cell::swap panic if the Cells partially overlap] (rust-lang/rust#114795) - [Reject invalid crate names in `--extern`] (rust-lang/rust#116001) - [Don't resolve generic impls that may be shadowed by dyn built-in impls] (rust-lang/rust#114941) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. None this cycle.

rustbot assigned thomcc Nov 11, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 11, 2022

rustbot assigned cramertj and unassigned thomcc Nov 12, 2022

rustbot assigned tmandry and unassigned cramertj Feb 13, 2023

tmandry added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 2, 2023

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 2, 2023

Clarify stability guarantee for lifetimes in enum discriminants

571b0fe

mkrasnitski force-pushed the discriminant-transmute-docs branch from d03bfad to 571b0fe Compare March 13, 2023 05:31

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Mar 14, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Mar 30, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 8, 2023

GuillaumeGomez mentioned this pull request Sep 8, 2023

Rollup of 6 pull requests #115672

Merged

bors merged commit 2cceedd into rust-lang:master Sep 8, 2023

rustbot added this to the 1.74.0 milestone Sep 8, 2023

mkrasnitski deleted the discriminant-transmute-docs branch November 9, 2023 18:22

lcnr reviewed Nov 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify stability guarantee for lifetimes in enum discriminants #104299

Clarify stability guarantee for lifetimes in enum discriminants #104299

mkrasnitski commented Nov 11, 2022 •

edited

Loading

rustbot commented Nov 11, 2022

rustbot commented Nov 11, 2022

CraftSpider commented Nov 11, 2022

thomcc commented Nov 12, 2022

mkrasnitski commented Nov 14, 2022 •

edited

Loading

mkrasnitski commented Feb 13, 2023

tmandry commented Mar 2, 2023

rfcbot commented Mar 2, 2023 •

edited

Loading

scottmcm commented Mar 7, 2023

mkrasnitski commented Mar 13, 2023

nikomatsakis commented Mar 14, 2023

rfcbot commented Mar 14, 2023

rfcbot commented Mar 24, 2023

mkrasnitski commented Apr 2, 2023

mkrasnitski commented Sep 8, 2023

oli-obk commented Sep 8, 2023

bors commented Sep 8, 2023

lcnr Nov 17, 2023

lcnr Nov 17, 2023

lcnr Nov 17, 2023

mkrasnitski Nov 17, 2023 •

edited

Loading

	/// The value of a [`Discriminant<T>`] is independent of any lifetimes in `T`. As such, reading
	/// The value of a [`Discriminant<T>`] is independent of any free lifetimes in `T`. As such, reading

Clarify stability guarantee for lifetimes in enum discriminants #104299

Clarify stability guarantee for lifetimes in enum discriminants #104299

Conversation

mkrasnitski commented Nov 11, 2022 • edited Loading

rustbot commented Nov 11, 2022

rustbot commented Nov 11, 2022

CraftSpider commented Nov 11, 2022

thomcc commented Nov 12, 2022

mkrasnitski commented Nov 14, 2022 • edited Loading

mkrasnitski commented Feb 13, 2023

tmandry commented Mar 2, 2023

rfcbot commented Mar 2, 2023 • edited Loading

scottmcm commented Mar 7, 2023

mkrasnitski commented Mar 13, 2023

nikomatsakis commented Mar 14, 2023

rfcbot commented Mar 14, 2023

rfcbot commented Mar 24, 2023

mkrasnitski commented Apr 2, 2023

mkrasnitski commented Sep 8, 2023

oli-obk commented Sep 8, 2023

bors commented Sep 8, 2023

lcnr Nov 17, 2023

Choose a reason for hiding this comment

lcnr Nov 17, 2023

Choose a reason for hiding this comment

lcnr Nov 17, 2023

Choose a reason for hiding this comment

mkrasnitski Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

mkrasnitski commented Nov 11, 2022 •

edited

Loading

mkrasnitski commented Nov 14, 2022 •

edited

Loading

rfcbot commented Mar 2, 2023 •

edited

Loading

mkrasnitski Nov 17, 2023 •

edited

Loading